-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Define Bazel version in one shared location #1252
Conversation
2793e37
to
8bbc94a
Compare
# installation, see https://github.com/NixOS/nixpkgs/issues/80950. | ||
VERSION_EXPECTED="bazel $(cat "$DIR/bazelversion")" | ||
VERSION_ACTUAL=$(bazel version --gnu_format) | ||
# nixpkgs Bazel version ends on '- (@non-git)'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we fix this in nixpkgs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would also fix this issue, so that would be good. Though, I don't think we need to block this PR on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that’s already fixed by the changes in NixOS/nixpkgs#80739 (review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC that PR changes bazel-real
for bazel-VERSION...
, so it fixes NixOS/nixpkgs#80950, but doesn't change the - (@non-git)
part. (Unless Bazel 2.1 does that, I haven't checked).
Instead of each CI pipeline separately defining the Bazel version, we define it in one shared location `.ci/bazelversion`. Ideally, we would use `.bazelversion` in the top-level which would be compatible with [bazelisk](https://github.com/bazelbuild/bazelisk). Unfortunately, this is incompatible with the way Bazel is packaged in nixpkgs right now. See, `.ci/check-bazel-version`. The pipelines that use nixpkgs to provision Bazel check that the Bazel version matches using `.ci/check-bazel-version`. The pipelines that fetch a Bazel binary distribution use `.ci/fetch-bazel-bindist` which in turn consults the `.ci/bazelversion` file. To avoid issues with corrupted downloads we check the downloaded bindist against expected sha256 hashes defined in `.ci/bazel-sha256`. The files `.ci/bazelversion` and `.ci/bazel-sha256` need to be kept in sync. The script `.ci/update-bazel-version` helps with this. Given a Bazel version number it will update both files. The nixpkgs revision needs to be updated separately. The `MAINTAINERS.md` file has been updated accordingly.
To work around this we store separate `.sha256` files for each platform.
git was automatically converting `\n` lineendings to `\r\n`, which confused `sha256sum`. By declaring `.sha256` files as binary `.gitattributes` we avoid this issue.
Instead of each CI pipeline separately defining the Bazel version, we define it in one shared location
.ci/bazelversion
. Ideally, we would use.bazelversion
in the top-level which would be compatible with bazelisk. Unfortunately, this is incompatible with the way Bazel is packaged in nixpkgs right now. See NixOS/nixpkgs#80950.The pipelines that use nixpkgs to provision Bazel check that the Bazel version matches using
.ci/check-bazel-version
. This was already the case on Linux, but has been added on macOS. The pipelines that fetch a Bazel binary distribution use.ci/fetch-bazel-bindist
which in turn consults the.ci/bazelversion
file. This was already the case on buildkite, but has been added to netlify and Azure. To avoid issues with corrupted downloads we check the downloaded bindist against expected sha256 hashes defined in.ci/bazel-*.sha256
.The files
.ci/bazelversion
and.ci/bazel-*.sha256
need to be kept in sync. The script.ci/update-bazel-version
helps with this. Given a Bazel version number it will update both files. The nixpkgs revision needs to be updated separately. TheMAINTAINERS.md
file has been updated accordingly.